Skip to content

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Sep 12, 2025

When converting pixels to point there can be depending on the zoom level be a fractional result.
Currently in all cases these result is converted into an integer using Math.round() that will make values +/-0.5 resulting in small values to be round towards a smaller value (see here).

While it is maybe valid for a location, when using points to express a dimension this is not okay as it will result in the reported (integer) value to be to small leading to errors when the SWT API is then used after performing additional computations maybe.

This now makes the following adjustments:

  1. Introduce a rounding mode that allows different ways of rounding and adds as a first step ROUND (the previous default) and UP (for always round towards the largest integer)
  2. Adjust the Control class to decide what mode is best in what situation.

See

@laeubi laeubi marked this pull request as draft September 12, 2025 07:31
@laeubi laeubi mentioned this pull request Sep 12, 2025
Copy link
Contributor

github-actions bot commented Sep 12, 2025

Test Results

0 files   -   118  0 suites   - 118   0s ⏱️ - 10m 43s
0 tests  - 4 435  0 ✅  - 4 418  0 💤  - 17  0 ❌ ±0 
0 runs   -   298  0 ✅  -   294  0 💤  -  4  0 ❌ ±0 

Results for commit 0505446. ± Comparison against base commit 5d2665d.

♻️ This comment has been updated with latest results.

@amartya4256
Copy link
Contributor

As discussed here #2381 (comment), we need to think of a better approach of using these rounding methods inside the OfFloat classes.

@amartya4256
Copy link
Contributor

@laeubi Could you propose a better way of hiding this rounding detail away from consumers?

@laeubi
Copy link
Contributor Author

laeubi commented Sep 24, 2025

@laeubi Could you propose a better way of hiding this rounding detail away from consumers?

I'm not sure what kind of "hiding" is needed here... only the caller can know what rounding mode is required/desired (unless we agree to always round up). Also as this is all internal API I assume the few consumers of it should be well aware of what they get.

@amartya4256 amartya4256 linked an issue Sep 24, 2025 that may be closed by this pull request
@akoch-yatta
Copy link
Contributor

@laeubi Having a short look into this issue, I think, it would make sense to push that idea for M1. Independent of internal adaptions, if I understand it correctly, two things are missing:

  1. Evaluate all calls to Wint32DPIUtils#pixelToPoint(Point, int) and (at best) replace them by passing the desired RoundingMode
  2. Properly test it

Am I missing something?

As M1 is just right around the corner, it would be necessary to put some effort into it soon. Question is, do you wanna bring this PoC into a mergeable state or could someone else take over your idea in this PR and do it themselves?

When converting pixels to point there can be depending on the zoom level
be a fractional result.
Currently in all cases these result is converted into an integer using
`Math.round()` that will make values `+/-0.5` resulting in small values
to be round towards a smaller value.

While it is maybe valid for a _location_, when using points to express a
_dimension_ this is not okay as it will result in the reported (integer)
value to be to small leading to errors when the SWT API is then used
after performing additional computations maybe.

This now makes the following adjustments:

1. Introduce a rounding mode that allows different ways of rounding and
adds as a first step ROUND (the previous default) and UP (for always
round towards the largest integer)
2. Adjust the `Control` class to decide what mode is best in what
situation.

See
- eclipse-platform#2381
- eclipse-platform#2166
@laeubi
Copy link
Contributor Author

laeubi commented Sep 24, 2025

@akoch-yatta Yes I think your summary is correct.

I would be fine if someone would use that to bring int into a mergable state, I more created it as a base for testing and discussion, e.g. to see if it fixes the cut-off problem, and I have only investigates some calls in the obvious Control part here.

@amartya4256
Copy link
Contributor

What I understand is that since Point is used for 2 semantics (Location and Size), there are different rounding modes needed for Location and Size. Can we validate that this is true for every case? If so I think we can provide separate scaling methods for size and location which would then use different rounding modes.

@HeikoKlare
Copy link
Contributor

@laeubi can we close this as superceded by ..?

@laeubi laeubi closed this Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Labels are cut off by autoscaling
4 participants